Skip to content

use cross platform path separatator in build #5085

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 10, 2018

Conversation

martijnhoekstra
Copy link
Contributor

Hello, world!

build instructions now work on Windows.

Copy link
Member

@dottybot dottybot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

@@ -453,7 +453,7 @@ object Build {
def findLib(attList: Seq[Attributed[File]], name: String) = attList
.map(_.data.getAbsolutePath)
.find(_.contains(name))
.toList.mkString(":")
.toList.mkString(java.io.File.pathSeparatorChar.toString)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pathSeparator can be used instead of pathSeparatorChar.toString.

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're at it, you may want to grep for ":" in the codebase, there's quite a few place where we should be use pathSeparator too.

@martijnhoekstra martijnhoekstra force-pushed the winbuild branch 2 times, most recently from afa7b6b to 6379ecf Compare September 7, 2018 07:56
@martijnhoekstra
Copy link
Contributor Author

I think I caught all platform-incompatible uses of ':' and ":" as path separators. There may be more lurking somewhere as parts of hardcoded strings.

@smarter smarter merged commit 0c88214 into scala:master Sep 10, 2018
@smarter
Copy link
Member

smarter commented Sep 10, 2018

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants